Skip to content

Initial KeyVaultAccessControlClient for Java#12405

Merged
vcolin7 merged 43 commits into
masterfrom
users/vicolina/keyvault-rbac
Sep 4, 2020
Merged

Initial KeyVaultAccessControlClient for Java#12405
vcolin7 merged 43 commits into
masterfrom
users/vicolina/keyvault-rbac

Conversation

@vcolin7

@vcolin7 vcolin7 commented Jun 23, 2020

Copy link
Copy Markdown
Member

Tests and samples are still pending and the README is a work in progress.

Based on issue: #8007.

@vcolin7 vcolin7 added KeyVault Client This issue points to a problem in the data-plane of the library. labels Jun 23, 2020
@vcolin7 vcolin7 added this to the [2020] July milestone Jun 23, 2020
@vcolin7 vcolin7 requested review from g2vinay and heaths June 23, 2020 00:11
@vcolin7 vcolin7 self-assigned this Jun 23, 2020
@vcolin7 vcolin7 force-pushed the users/vicolina/keyvault-rbac branch from b0635bc to 0b1fd31 Compare June 24, 2020 20:58
@vcolin7 vcolin7 requested a review from christothes June 25, 2020 18:11
@vcolin7 vcolin7 marked this pull request as ready for review June 25, 2020 18:11

@JimSuplizio JimSuplizio left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a tag that needs to be updated in the README that I called out but otherwise the EngSys items in here are fine. I trust you to update the tag so I'll pre-approve this from an EngSys perspective.

Comment thread sdk/keyvault/azure-security-keyvault-administration/README.md Outdated

@heaths heaths left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some parameter names different from other languages, but we can unify on those for beta 2. Same for a few getters I pointed out.

Comment thread eng/jacoco-test-coverage/pom.xml Outdated
* @param principalId The principal ID assigned to the role. This maps to the ID inside the Active Directory.
* It can point to a user, service principal, or security group.
*/
public KeyVaultRoleAssignmentProperties(String roleDefinitionId, String principalId) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are required parameters for create methods. Are they being passed to those methods directly as well?

@vcolin7 vcolin7 Sep 2, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These parameters are passed directly to the service methods.

This Properties object is based on how the service Swagger groups parameters, however, I believe we certainly could change the method signature to take all required values like this:

public Mono<KeyVaultRoleAssignment> createRoleAssignment(
    KeyVaultRoleAssignmentScope roleScope,
    String roleDefinitionId,
    String principalId)

Instead of what we have today:

public Mono<KeyVaultRoleAssignment> createRoleAssignment(
    KeyVaultRoleAssignmentScope scope,
    KeyVaultRoleAssignmentProperties properties)

What do you think @heaths?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we were doing it that way in .NET after a discussion, but I don't see that in source. @christothes and @sadasant, was it a different method we were talking about pulling roleDefinitionId and principalId out of, or was that for the model constructor? I may just be remembering wrong here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we still use a properties bag in .NET, but I like the individual parameter shape better.

Comment thread sdk/keyvault/azure-security-keyvault-administration/README.md Outdated
Comment thread sdk/keyvault/azure-security-keyvault-administration/pom.xml
@vcolin7 vcolin7 force-pushed the users/vicolina/keyvault-rbac branch from 9ed0f6f to 5a208bf Compare September 3, 2020 22:09
@vcolin7 vcolin7 force-pushed the users/vicolina/keyvault-rbac branch from 8ede798 to ca96cc7 Compare September 4, 2020 06:59
@vcolin7 vcolin7 force-pushed the users/vicolina/keyvault-rbac branch from ca96cc7 to a476010 Compare September 4, 2020 07:03
@vcolin7 vcolin7 merged commit 5eb4798 into master Sep 4, 2020
@vcolin7 vcolin7 deleted the users/vicolina/keyvault-rbac branch September 4, 2020 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Client This issue points to a problem in the data-plane of the library. KeyVault

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants